[OTLP] Add support for GZip compression#7055
[OTLP] Add support for GZip compression#7055martincostello merged 21 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7055 +/- ##
==========================================
+ Coverage 89.26% 89.51% +0.25%
==========================================
Files 271 272 +1
Lines 13088 13296 +208
==========================================
+ Hits 11683 11902 +219
+ Misses 1405 1394 -11
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Pull request overview
Adds configurable gzip compression support to the OTLP exporter to align with the OTLP exporter specification (including env var configuration) and validates behavior via new/updated unit tests.
Changes:
- Introduces
OtlpExportCompressionandOtlpExporterOptions.Compression, with spec env var support (OTEL_EXPORTER_OTLP*_COMPRESSION). - Implements gzip request compression for OTLP over HTTP and gRPC export clients.
- Adds/updates unit tests and updates changelog + public API tracking files.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpSpecConfigDefinitionTests.cs | Extends spec config test data and assertions to include compression env vars. |
| test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpHttpExportClientTests.cs | Adds HTTP export tests validating payload and headers with/without gzip. |
| test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpGrpcExportClientTests.cs | Adds gRPC export tests validating compressed framing and decompression behavior. |
| test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpExporterOptionsTests.cs | Extends options/config tests to cover compression env var parsing and invalid values. |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs | Adds Compression option, parsing helper, and spec-env-var application. |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExportCompression.cs | New public enum defining supported compression modes. |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/IOtlpExporterOptions.cs | Adds Compression to internal options contract. |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpSpecConfigDefinitions.cs | Adds spec env var constants for compression across signals. |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/ExportClient/OtlpHttpExportClient.cs | Implements gzip compression for HTTP request content. |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/ExportClient/OtlpGrpcExportClient.cs | Implements gzip compression for gRPC framing + request header. |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/ExportClient/OtlpExportClient.cs | Centralizes content creation via overridable method; wires CompressionEnabled. |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md | Documents new opt-in gzip compression configuration. |
| src/OpenTelemetry.Exporter.OpenTelemetryProtocol/.publicApi/Stable/PublicAPI.Unshipped.txt | Tracks new public API surface (OtlpExportCompression, OtlpExporterOptions.Compression). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add support for GZip compression to the OTLP exporter. Picks up from open-telemetry#6494. Resolves open-telemetry#3961. Co-Authored-By: Hannah Haering <157852144+hannahhaering@users.noreply.github.com>
- Avoid allocating a buffer for every write of compressed gRPC data. - Simplify `IsTransientNetworkError()`.
Add coverage for different ways to specify the compression value.
- Ensure only one `grpc-encoding` header. - Remove redundant null checks.
Remove trailing spaces.
Add missing blank line.
|
|
||
| * Added opt-in support for gzip compression. Compression can be configured | ||
| programmatically via the new `OtlpExporterOptions.Compression` property, | ||
| or through the environment variables such as `OTEL_EXPORTER_OTLP_COMPRESSION=gzip`. |
There was a problem hiding this comment.
nit: maybe mention the signal specific env vars too here.
There was a problem hiding this comment.
also, if we can update the OTLP's readme also, that'd better.
There was a problem hiding this comment.
Good point - I forgot about the README.
| return base.CreateHttpContent(buffer, contentLength); | ||
| } | ||
|
|
||
| var compressedStream = new MemoryStream(); |
There was a problem hiding this comment.
lets leave a todo for a future optimization to see if we can avoid allocation by renting/pooling arrays.
There was a problem hiding this comment.
What specifically were you thinking of?
- Renting a buffer to pass into the
MemoryStream? - Renting a single shared buffer (like the main exporter does) for compression and re-using it across requests to pass into the
MemoryStream? - A custom
Streamimplementation that acts like a memory stream but is backed by rented buffers? - Something else?
There was a problem hiding this comment.
I've got an implementation of the third option locally, I'm just testing it.
There was a problem hiding this comment.
https://github.com/communityToolkit/dotnet has quite a few implementations.
There was a problem hiding this comment.
We wouldn't be able to take a dependency on those.
There was a problem hiding this comment.
I wouldn't recommend that for this library either.
But it has quite a few with unit tests.
I don't see a problem grabbing the code from one that fits here.
There was a problem hiding this comment.
I took a quick look, and didn't see an "obvious" one that grabbed from the array pool, plus they seemed to have a bunch of extra interfaces I'd have to prune out. As I'd already written an implementation when you commented, I'll stick with that unless there's a compelling reason to replace it with something adapted and vendored from there.
There was a problem hiding this comment.
Oops! I meant to leave a TODO for revisiting this in a future PR to keep the initial version easier to review. My suggestion is to avoid the optimization in this PR, so review can focus on just the feature, and then a follow up where we can focus on pooling/efficiency part!
Sorry I was not very clear initially!
There was a problem hiding this comment.
As I've already done the work either I can remove it and just immediately open a PR as a draft that's the same plus 2 extra files to rebase later and then re-run the benchmarks without the optimization, or we can just leave it in.
Log the compressed and uncompressed sizes when using GZip.
Add an implementation of `Stream` that uses pooled buffers.
Rename to `GZip` to match `GZipStream`.
Add documentation for compression.
Add a benchmark for OTLP compression.
Benchmark results
|
Stub-out the `HttpClient` requests.
Use a top-level program for benchmarks and return whether they have succeeded to the caller.
`Stream.CanTimeout` is already `false`.
Add coverage for `PooledBufferStream` that isn't already covered by other tests.
Rename `buf` to `rented` and fix trying to return an empty buffer.
Log the compression type in the event source.
Update PR number.
Fixes #3961
Changes
Add support for GZip compression to the OTLP exporter.
Continues the work started in #6494.
Once released, should also update the compliance matrix.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes